Skip to content

fix: Report fails to render with pytest-xdist (1) #591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix: Report fails to render with pytest-xdist (1) #591

wants to merge 1 commit into from

Conversation

BeyondEvil
Copy link
Contributor

@BeyondEvil BeyondEvil commented Mar 10, 2023 •
edited
Loading

The reason why the report fails to render when running with pytest-xdist, is that it adds a node attribute with a WorkerController object as its value.

This object is unserializable, so it fails when trying to convert the test data to JSON.

This is proposed solution 1.

Solution 2: #592
Solution 3: #593

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

This was referenced Mar 10, 2023
Closed
Closed
@BeyondEvil BeyondEvil changed the title fix: Report fails to render with pytest-xdist fix: Report fails to render with pytest-xdist (1) Mar 10, 2023
@nicoddemus
Copy link
Member

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

IMHO the WorkerController is an implementation detail and of little to no value to users, so I don't see an advantage in serializing it. 👍

@BeyondEvil BeyondEvil removed the request for review from ssbarnea March 10, 2023 15:08
@BeyondEvil
Copy link
Contributor Author

There's also the option of making the WorkerController class serializable upstream. Ping @nicoddemus

IMHO the WorkerController is an implementation detail and of little to no value to users, so I don't see an advantage in serializing it. 👍

By extension then, I guess there's no point in passing it in the JSON to the report either? (invalidating solution 3 #593 )

@BeyondEvil BeyondEvil requested a review from ssbarnea March 10, 2023 15:10
@nicoddemus
Copy link
Member

Possibly, but more investigation would be needed.

@BeyondEvil
Copy link
Contributor Author

Possibly, but more investigation would be needed.

I think it's safe to keep it out of the report.

It wasn't part of the legacy report afaict. And if someone has a use case for having it, we can re-add it easily.

@RonnyPfannschmidt RonnyPfannschmidt mentioned this pull request Mar 11, 2023
Open
RonnyPfannschmidt
RonnyPfannschmidt requested changes Mar 11, 2023
View reviewed changes
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is lossy in the face of valid extensions, os i recommend against it

@BeyondEvil BeyondEvil mentioned this pull request Mar 19, 2023
Merged
@BeyondEvil
Copy link
Contributor Author

Fixed by: #598

@BeyondEvil BeyondEvil closed this Mar 19, 2023
@BeyondEvil BeyondEvil deleted the beyondevil/fix-xdist-1 branch March 19, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt requested changes

@ssbarnea ssbarnea Awaiting requested review from ssbarnea

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BeyondEvil @nicoddemus @RonnyPfannschmidt